-
Notifications
You must be signed in to change notification settings - Fork 28.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
n-api: implement napi_run_script #15216
Conversation
doc/api/n-api.md
Outdated
``` | ||
|
||
- `[in] env`: The environment that the API is invoked under. | ||
- `[in] script`: The string containing the script to execute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it clear that this is in UTF-8? Or just take a string napi_value instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be easier to take a napi_value
instead.
doc/api/n-api.md
Outdated
``` | ||
|
||
- `[in] env`: The environment that the API is invoked under. | ||
- `[in] script`: The string containing the script to execute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… encoded as UTF-8 and null-terminated?
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL)); | ||
|
||
NAPI_CALL(env, napi_get_value_string_utf8(env, arg, NULL, 0, &string_size)); | ||
string = (char *)malloc(string_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don’t we technically need to free()
this?
@@ -215,12 +216,35 @@ napi_value testAdjustExternalMemory(napi_env env, napi_callback_info info) { | |||
return result; | |||
} | |||
|
|||
napi_value testNapiRun(napi_env env, napi_callback_info info) { | |||
size_t string_size; | |||
char *string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointer star goes with the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’re inconsistent about that in this file, and I think this can pass as “idiomatic for C code”
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL)); | ||
|
||
NAPI_CALL(env, napi_get_value_string_utf8(env, arg, NULL, 0, &string_size)); | ||
string = (char *)malloc(string_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
doc/api/n-api.md
Outdated
- `[in] script`: The string containing the script to execute. | ||
- `[out] result`: The value resulting from having executed the script. | ||
|
||
[Script Execution]: #n_api_script_execution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order
679ade8
to
402d5be
Compare
Switched script parameter to |
07920bb
to
c7d03d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future we might want to have an equivalent for V8's Script or UnboundScript, but this LGTM and should suffice for now.
doc/api/n-api.md
Outdated
@@ -3556,6 +3557,25 @@ NAPI_EXTERN napi_status napi_is_promise(napi_env env, | |||
- `[out] is_promise`: Flag indicating whether `promise` is a native promise | |||
object - that is, a promise object created by the underlying engine. | |||
|
|||
## Script execution | |||
|
|||
N-API allows you to execute a string containing JavaScript using the underlying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid the use of you
in the docs.
N-API allows the execution of a JavaScript string using the underlying JavaScript engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once comment from @jasnell is addressed
c7d03d4
to
ca0d064
Compare
@jasnell I have reworded the sentence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
SmartOS re-run: https://ci.nodejs.org/job/node-test-commit-smartos/11353/ |
Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Landed in 61e9ba1. |
Fixes: nodejs/abi-stable-node#51 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Fixes: nodejs/abi-stable-node#51 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Fixes: nodejs/abi-stable-node#51 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@gabrielschulhof sorry, missed reviewing this PR. @MSLaguana and I were looking at it for implementation in node-chakracore and we had a question- if someone was debugging their script and a script executed from this API was on the stack- what is the "filename" they expect to see? Dynamic code or something equivalent? Should there be a way of modules identifying themselves to the debugger? Or should these frames be considered "library code" and hidden from the debugger? |
@digitalinfinity maybe we should treat it like an eval. Chrome developer
tools opens a new tab and calls it "VM<number>" where number is probably
some internal counter or something like that. It paints the tab a different
colour.
In fact, I ran `node --inspect-brk
test/addons-napi/test_general/testNapiRun.js` and when I stepped into
`addon.testNapiRun()` it behaved exactly like `eval()`.
…On Fri, Sep 15, 2017 at 2:49 AM, Hitesh Kanwathirtha < ***@***.***> wrote:
@gabrielschulhof <https://github.com/gabrielschulhof> sorry, missed
reviewing this PR. @MSLaguana <https://github.com/mslaguana> and I were
looking at it for implementation in node-chakracore and we had a question-
if someone was debugging their script and a script executed from this API
was on the stack- what is the "filename" they expect to see? Dynamic code
or something equivalent? Should there be a way of modules identifying
themselves to the debugger? Or should these frames be considered "library
code" and hidden from the debugger?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15216 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA7k0etVgsKC-XXzJmDMQLQoyJOln7O1ks5sibt6gaJpZM4POMQh>
.
|
Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Fixes: nodejs/abi-stable-node#51 Backport-PR-URL: #19447 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: #616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: #616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: nodejs/node-addon-api#616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: nodejs/node-addon-api#616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: nodejs/node-addon-api#616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: nodejs/node-addon-api#616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: nodejs/node-addon-api#616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: nodejs/abi-stable-node#51
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
n-api